Eliminate mocking in net device #4802
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Clippy insists and my pre-commit hook keeps trying to add this to random commits. Signed-off-by: Patrick Roy <[email protected]>
roypat
force-pushed
the
no-net-mocking
branch
from
f1a4e36 to
c5003a0
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4802 +/- ##
==========================================
+ Coverage 84.31% 84.35% +0.03%
==========================================
Files 249 249
Lines 27521 27512 -9
==========================================
+ Hits 23205 23207 +2
+ Misses 4316 4305 -11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
roypat
force-pushed
the
no-net-mocking
branch
5 times, most recently
from
3fc4e71 to
ad6a859
Compare
roypat
added
the
Status: Awaiting review
roypat
requested review from
ShadowCurse and
bchalios
and removed request for
bchalios
The test code has two ways to inject a packet to be read from the tap device: Using the `TapTrafficSimulator`, which actually uses `sendto` to send something through the tap device, or `MockFrame`, which hijacks `Net::read_tap` to not actually read from the tap and just return the packet. Eliminate the latter, as it was only used in a single test, and needless complicates the test setup (well, arguably, unittests shouldn't use actual Tap devices, and thus we should eliminte `TapTrafficSimulator` instead, but I'm choosing my battles today). Do the injection of the packet before we deplete the ratelimiter, as otherwise the injection takes long enough for the ratelimiter to replenish, causing the test to fail intermittently. Also increase the replenish time. Signed-off-by: Patrick Roy <[email protected]>
The only use of this implement was its test. Remove both. Signed-off-by: Patrick Roy <[email protected]>
There was a very elaborate mocking setup in the net device code involving `cfg(not(test))`, but the only point of it all was to be able to test the scenario where `tap.write_iovec` or `tap.read` return an error. We do not need mocking to achieve this though: We can simply close the tap fd. Thus remove all the mocking, and update the negative tests to close the fd. I had half a mind to just delete the tests, as currently we are not sensibly handling errors in device emulation anyway, so the assertions these tests make aren't exactly useful. However, once we implement device reset, this will probably change, so let's keep the tests. Signed-off-by: Patrick Roy <[email protected]>
This function used an unsound `mem::transmute` to invalidly extend a lifetime with the goal of creating a self-referential struct. Sadly, after returning from `get_default`, the `GuestMemoryMmap` gets moved to a different stack frame, and so all references that this function created become dangling pointers. Fix this by not trying to create self-referential struct. Signed-off-by: Patrick Roy <[email protected]>
roypat
force-pushed
the
no-net-mocking
branch
from
ad6a859 to
229a2fe
Compare
ShadowCurse
approved these changes
Sep 16, 2024
kalyazin
approved these changes
Sep 16, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
Removes mocking in net device code. Mocking in rust should not be done using
cfg(not(test)), but rather using traits. However, in this specific case, there is no need for mocking, because we already have a separate framework for simulating tap traffic by writing to a socket that is connected to an actual tap device. So just remove mocking and switch the 3 tests that relied on it to this other framework.Reason
Removes the last major source of
cfg(not(test))in our code base, and will make @bchalios's job easier (or so I've been told)License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.PR Checklist
PR.
CHANGELOG.md.TODOs link to an issue.contribution quality standards.
rust-vmm.